Full Code Review for Open Source Approval by hkasemir · Pull Request #11 · udacity/fluent-react-utils

hkasemir · 2018-12-11T21:24:12Z

To be able to officially run through the code and ensure quality and cleanliness of code - that we are not exposing any proprietary information.

fix bad import path

split into two packages using lerna for management

Hkasemir/circleci

move constants out of shared folder

dcwither

Everything overall looks good. There's one babel thing to double check since IIRC not all browsers support Object.assign, and a few minor suggestions that might be worth responding to before merge, and another few longer term suggestions that definitely shouldn't delay release.

dcwither · 2018-12-18T21:55:07Z

+   Copyright 2018 Udacity

-Copyright (c) 2018 Dineshkumar Pandiyan <flexdinesh@gmail.com>
+   Licensed under the Apache License, Version 2.0 (the "License");


Is this what we want all our work licensed under? I'm kinda curious why not MIT - though they're both very permissive

ultimately we decided to use this because it is what the base fluent package is licensed under, and so it made sense to use the same one since they will be used together.

dcwither · 2018-12-18T21:56:28Z

+{
+  "env": {
+    "development": {
+      "presets": ["env", "react", "es2015"],


shouldn't need es2015 along side env afaik

dcwither · 2018-12-18T21:57:02Z

+      <Localized id='NotAuthorized_oopsMessage'>
+        <h1>
+          {/* here is a comment for the localizer */}
+          Oops! looks like you're not authorized to use this app.


nit: capitalize?

dcwither · 2018-12-18T21:57:15Z

+    <div className="container">
+      <Loc.H1 l10nId="NotAuthorized_oopsMessage">
+        {/* here is a comment for the localizer */}
+        Oops! looks like you're not authorized to use this app.


nit: capitalize

dcwither · 2018-12-18T22:01:32Z

+`l10nJsx` props.
+
+
+### `augmentLoc(customElements)`


AFAICT this isn't exactly an "augmentation" since it's returning something new. On the one hand, it's just as easy for someone to do {...Loc, MyButton: MyLocalizedButton}, it is good for communicating how to use it.

I think it would be helpful to add to the docs that this operation is pure

dcwither · 2018-12-18T22:02:37Z

+export function NotAuthorized() {
+  return (
+    <div className="container">
+      <T.H1 l10nId="NotAuthorized_oopsMessage">


Is this demonstrating the localization of a different prefix? Maybe to encourage best practices we should avoid single letter names?

dcwither · 2018-12-18T22:07:57Z

+  const varNames = Object.keys(l10nVars);
+
+  return varNames.reduce(
+    (vars, name) => Object.assign({}, vars, { [`$${name}`]: l10nVars[name] }),


can you make sure that the babelrc setup compiles this down to include the polyfill (but not in the global manner that @babel/polyfill does?

https://babeljs.io/docs/en/babel-plugin-transform-runtime I think is the transform to use.

Why not just use spreads here?

(vars, name) => {...vars, [`$${name}`]: l10nVars[name] },

dcwither · 2018-12-18T22:10:31Z

@@ -0,0 +1,80 @@
+import React from 'react';


Suggestion to keep the tests in the same folders as the things they're testing. I've come to prefer that, and I think services team does as well. @hitchcockwill?

dcwither · 2018-12-18T22:11:05Z

+// modified from https://searchfox.org/mozilla-central/source/intl/l10n/L10nRegistry.jsm
+// removed option for 'flipped' to test RTL
+
+const ACCENTED_MAP = {


Does this end up in live code? Should we ensure it doesn't?

That would be at the discretion of projects using this package to determine how they set up the string transformation settings to use pseudolocalization, and whether it is accessible in live environments or not.

dcwither · 2018-12-18T22:13:12Z

+}
+
+function getEntryName(entry = {}) {
+  if (entry.id) {


is it possible to entry to be null? that wouldn't be caught by the entry = {}

Same with the above functions. Typescript could help our confidence here.

hitchcockwill

I can take a little deeper dive later this week. One thing that would be good to work on is better test coverage for the extractions package. There are a ton of small units of code there that could be easily tested but don't have coverage right now.

hitchcockwill · 2018-12-19T00:23:24Z

+import { STANDARD_ELEMENT_TYPES } from './constants';
+
+export const Loc = {
+  A: makeLocalizedElement('a'),


What's the reason for capitalizing these? Why not match the actual element name exactly?

Interesting idea!

My reasoning for the capitalization was to make clear that it is a React component, and not just the basic html element. I think if we figure out a clear way to resolve this issue #9, then having the capitals will be particularly beneficial to understand that it's different. If we don't, I could see the argument for lowercase, since it would just be <Loc.a>. Though l still feel like it goes against React best practices

hitchcockwill · 2018-12-19T00:25:05Z

+  const varNames = Object.keys(l10nVars);
+
+  return varNames.reduce(
+    (vars, name) => Object.assign({}, vars, { [`$${name}`]: l10nVars[name] }),


Why not just use spreads here?

(vars, name) => {...vars, [`$${name}`]: l10nVars[name] },

hitchcockwill · 2018-12-19T00:34:45Z

+    "babel-preset-es2015": "^6.24.1",
+    "babel-preset-minify": "^0.3.0",
+    "babel-preset-react": "^6.24.1",
+    "chai": "^4.1.2",


Is there any particular reason for using chai and mocha for testing? Why not jest?

hitchcockwill · 2018-12-19T00:36:39Z

+    "test:prod": "cross-env BABEL_ENV=production npm run test",
+    "test:only": "mocha --require babel-core/register --require babel-polyfill --recursive",
+    "cover": "istanbul cover _mocha -- --require babel-core/register --require babel-polyfill --recursive",
+    "lint": "eslint src test",


Consider adding prettier.

hitchcockwill · 2019-01-02T19:22:23Z

@@ -0,0 +1,18 @@
+{


Do we need to have separate .babelrc and .eslintrc files for each package? Is there any way to have a single of each rc file at the root level?

hkasemir added 30 commits September 18, 2018 10:15

a cli that does something

90b5ea1

add some organization and rcfile abilities

4271a5a

working CLI for jsx components

bf7974a

add Loc components

438afc3

allow using jsx and object spreads

02a09da

add logging for undefined localizationKeys

786746d

add export for components

378691c

clear out tests

ad180a5

add pseudolocalize function

c993757

reorganize folders

2ed1c6d

update readme docs

a3b548c

clean up CLI, add some self-documentation and options

931897c

apply apache-2.0 license

c8e9d69

fix typo in Localized id

bd361de

add repo and keywords to package.json

f8eb8d4

add some tests

7ce1a27

add tests for makeLocalizedElement

1d44c32

add tests for augmentLoc

e4c430b

add test for getSourceStrings

8c31ce6

add tests for extracting shorthand

65e4718

remove unnecesary / useless things

d771801

clean up

8f97327

add info about comments

ee42465

cleanup

9891199

is not acutally on npm yet

785fec8

initial file restructure

9eb942b

update broken file paths to successfully bootstrap and test

43e2412

fix bad import path

update readmes

4539f79

add codeowners file

60722f1

clean out package.json in root

f9af06b

hkasemir added 8 commits November 20, 2018 12:52

Merge pull request #2 from udacity/hkasemir/use-lerna

c89a992

split into two packages using lerna for management

v0.1.1

5c8ecee

include basic circleci file

40818a7

install lerna as sudo

cb3ad84

Merge pull request #6 from udacity/hkasemir/circleci

368c770

Hkasemir/circleci

move constants out of shared folder

7b73870

Merge pull request #10 from udacity/hkasemir/standard-element-constants

8c48196

move constants out of shared folder

v0.1.2

944c59b

hkasemir requested review from dcwither and hitchcockwill December 11, 2018 21:25

hkasemir assigned hitchcockwill and dcwither Dec 11, 2018

include npm install instructions

4cecde0

hkasemir force-pushed the master branch from c7cc009 to 4cecde0 Compare December 13, 2018 23:43

dcwither approved these changes Dec 18, 2018

View reviewed changes

dcwither assigned hkasemir and unassigned dcwither Dec 18, 2018

hitchcockwill reviewed Dec 19, 2018

View reviewed changes

hitchcockwill reviewed Jan 2, 2019

View reviewed changes

hkasemir closed this Sep 5, 2019

		`l10nJsx` props.


		### `augmentLoc(customElements)`

Conversation

hkasemir commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcwither left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hitchcockwill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hkasemir commented Dec 11, 2018 •

edited

Loading